-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding hack for allowSharedKeyAccess = false
on storage accounts
#3595
Conversation
@@ -541,7 +541,7 @@ | |||
}, | |||
"location": "[resourceGroup().location]", | |||
"name": "[substring(parameters('storageAccountDomain'), 0, indexOf(parameters('storageAccountDomain'), '.'))]", | |||
"type": "Microsoft.Storage/storageAccounts", | |||
"type": "Microsoft.Storage/storageAccounts", { "properties": "allowSharedAccessKey: false" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: I can add \n
to make this better indented. This was just a proof of concept / draft.
b, err := g.templateFixup(t) | ||
|
||
sharedAccessKeyHack := false | ||
if output == FileRPProduction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Nont and Weinong:
Without this if
, some other files besides rp-production.json
get modified, such as rp-oicd.json
, among others.
As I am not sure yet which set of the files we want to modify, I added this to help us control that.
I'm thinking probably all files, as we don't want any more SAS keys anywhere. But, still, being able to control it feels safer - I don't know if all the storage accounts have other authentication ways and are ready for SAS to be disabled.
b = bytes.ReplaceAll(b, []byte(`"type": "Microsoft.Storage/storageAccounts"`), []byte(`"type": "Microsoft.Storage/storageAccounts", { "properties": "allowSharedAccessKey: false" }`)) | ||
} | ||
|
||
// TO-DO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Weinong and Nont:
Added this so that we don't forget to consider doing the SDK migration instead of keeping this hack here for too long.
Also added a mention to this on the SDK migration wiki itself: https://msazure.visualstudio.com/AzureRedHatOpenShift/_wiki/wikis/AzureRedHatOpenShift.wiki/604062/Azure-SDK-track-2-migration
@@ -114,7 +114,13 @@ func (g *generator) Artifacts() error { | |||
} | |||
|
|||
func (g *generator) writeTemplate(t *arm.Template, output string) error { | |||
b, err := g.templateFixup(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: fix this due to the linter failure.
Abandoned because we won't be using this fix, we'll be updating the |
Which issue this PR addresses:
Fixes https://issues.redhat.com/browse/ARO-7791
What this PR does / why we need it:
We need to set
allowSharedAccessKey
property tofalse
on ourrp-production.json
ARM template. This PR changespkg/deploy/generator/templates.go
so that this happens when we runmake generate
.Test plan for issue:
Test steps described in Jira issue. Local test (seeing how
rp-production.json
looks like after runningmake generate
on my terminal) performed. INT tests and VMSS test yet to be done.Is there any documentation that needs to be updated for this PR?
Q: I guess not? Please correct me if I am wrong and/or teach me how I'd find out there isn't, so that I know this for the future.
How do you know this will function as expected in production?
We do need to make sure that, before rolling out this, we don't have any storage accounts relying on SAS that don't have an alternative authentication method.
Q: How could we do that?